fix(core,sdk): stop chat.agent losing messages during recovery - #4907
fix(core,sdk): stop chat.agent losing messages during recovery#4907ericallam wants to merge 12 commits into
Conversation
…ashboard reader
Adds TranscriptSnapshotV2 (entries of {id, final, message} plus an opaque state record) and parseTranscriptSnapshot(), which reads a version 1 or version 2 blob into the version 2 shape. The Sessions dashboard seeds its transcript through that reader, so it keeps rendering history once the SDK starts writing version 2. The SDK still writes version 1 in this change.
…ptStorage seam Introduces the TranscriptStorage interface (load/save over id-addressed changes: put, remove, truncateAfter, state) and makes the built-in snapshot writer its default implementation. The runtime keeps a shadow of the transcript it last saved and hands the storage the diff after every turn, failed turn and history-changing action, together with the stream cursors the next boot resumes from. The default storage reduces each changeset onto an in-memory copy and rewrites the blob as version 2, so a turn still costs one PUT and no GET, and the boot read goes through load. The snapshot read/write helpers move to their own module so the storage can import them without a cycle; the test seams keep their import path. The mock chat agent harness now reports version 2 snapshots and accepts either version as a seed.
🦋 Changeset detectedLatest commit: 2ece24d The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (41)
🧰 Additional context used📓 Path-based instructions (7)Always import from `@trigger.dev/sdk`.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Prefer static imports over dynamic imports.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging.📄 CodeRabbit inference engine (AGENTS.md) Files:
In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🔇 Additional comments (12)
WalkthroughThe session router now tracks recovered sequence claims and unsettled claims separately. It drops late live-tail records for claimed sequences and holds the resume floor until each claim settles. Chat recovery passes individual sequence numbers to the router and settles dispatched or undispatched recovered records. Tests cover router floor behavior and prevent duplicate recovered chat messages and turns. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Recovery now claims and settles individual messages to prevent message loss, but a gap in recovered sequences can still advance a replay cutoff too far in a narrowed recovery path, potentially suppressing a message. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 too large.) Full details: Description checkExplanation The description clearly explains the problem, fix, behavior, and test coverage. However, it omits the required issue reference, checklist, Testing section, Changelog section, and Screenshots section from the repository template. Resolution Add a "Closes #<issue>" reference, complete the checklist, and add the required Testing, Changelog, and Screenshots sections. State "N/A" for Screenshots if no screenshots apply, and include the commands or steps used to verify the change under Testing.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
7d29239 to
9a2cfbb
Compare
52addef to
1268dba
Compare
…nic session.in floor A snapshot with no savedAt no longer drops the whole transcript: savedAt is optional and defaults to 0 (it only orders snapshot history before live chunks, so losing it should not lose the conversation). The offline chat harness now models session.in as a durable stream whose seqNums stay monotonic across a chat's runs, and the test session-stream manager's setLastSeqNum only ever advances (matching the production manager). Without this a fresh per-run manager restarted seqNums at 0, so a continuation's follow-up message collided with the resume floor and was dropped, hanging the turn loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VG39FXXkFFU24U5EtJMwPi
…ript storage state The model lane after a compaction cannot be rebuilt from the transcript, so every continuation used to re-read the whole conversation and summarise it again. The runtime now records the compacted lane in the storage's state slot, with the transcript id it covers and a fingerprint of that prefix, and rebuilds from it at boot when the prefix is unchanged. A rollback or edit that reconverts the lane clears the state in the same changeset as the truncate. Conversational messages added with chat.inject are recorded the same way, anchored to the transcript message they followed, so they survive a continuation instead of living only in the worker that received them. Adds an in-memory storage that logs the changesets it receives, and a test-only override for the storage the runtime persists through, so the exact changesets for a turn, a mid-turn steer, a compaction, a rollback and an injection are asserted.
…t storage own the model's context One condition used to decide three things at boot: whether to read the persisted transcript, whether to replay the session's output tail, and whether to replay unacknowledged input. Registering hydrateMessages switched all three off, so an app that owned its own context also lost crash recovery, and no application can rebuild the tail its dead run had already emitted. The replays and onRecoveryBoot now run for every agent; only the transcript read is skipped for hydrateMessages. The storage can now declare loadContext, which the runtime calls on every turn and action in place of the accumulated transcript, the role hydrateMessages played, while save keeps receiving every change. hydrateMessages is deprecated with a one-time warning, and configuring it together with a storage that has loadContext is an error.
…nformance suite chat.agent takes a storage option, with the platform snapshot as the default and the TranscriptStorage types, defaultStorage, snapshotTranscriptStorage, memoryTranscriptStorage and reduceTranscriptChanges exported from @trigger.dev/sdk/ai. chat.createLoadTranscriptAction(storage) reads a conversation on the server for any storage, and useLoadTranscript renders it in the browser and seeds the transport's resume cursor. runTranscriptStorageTests from @trigger.dev/sdk/ai/test is the contract a storage implementation has to meet. A new secret-key endpoint, GET /api/v1/sessions/:id/transcript, pages the platform snapshot server-side so rendering the last page of a long conversation does not download all of it; the default storage uses it for paged reads and falls back to the whole blob.
… message rows The real-schema conformance target for the storage contract: one row per message in chat_messages, with the runtime's state and cursors in two new nullable columns on chats. Paging is by position in SQL. The agent itself still persists through its hooks.
…, and the changeset
Treat a non-positive page limit as no limit instead of an empty page a caller cannot tell apart from the end of the transcript. Hold a seeded resume cursor until the session exists, so a transcript load that resolves before the session is created still opens the live stream past the persisted history instead of replaying it.
Consume a pending resume cursor in setSession() and stop seedResumeCursor from moving an existing session's cursor backward, so a transcript load that resolves around session creation, or after the live stream already advanced, does not replay persisted events. Add conformance coverage for a partial answer's non-final status, and drive the resume-cursor unit tests through a real transport. Docs: loadContext also fires on actions, a custom storage is not limited to row-per-message backends, and the bring-your-own example imports anthropic.
2418fd0 to
af81d57
Compare
1268dba to
95d3ab5
Compare
saveTranscript made compaction and injections mutually exclusive in the persisted runtime state, but restoreModelLane treats them independently: injections anchored past a valid compaction apply on top, and if the compaction fingerprint fails to match at boot the whole compacted lane is discarded. In that case a same-turn injection that only lived in the dropped `injections` field was lost. Emit both fields; restore already filters injections that a valid compaction covers, so there is no double-apply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VG39FXXkFFU24U5EtJMwPi
af81d57 to
5f06b37
Compare
Rebased onto the transcript-storage stack. A continuation boot claims recovered session.in seqNums on the router and holds the resume cursor behind each until the boot settles it, so suppressing the tail's re-answer no longer advances the cursor past an un-answered message. Adds a changeset and router/boot tests.
95d3ab5 to
2ece24d
Compare
5f06b37 to
ed95a9e
Compare
Summary
When a chat.agent run boots to continue a session (a version handover, or a retry after a crash), it replays the unacknowledged user messages off
session.inand dispatches them itself. A previous change stopped the live tail from re-answering those same messages by folding them into the resume cursor in one step. That cursor is what the next boot reads to know where to resume, and folding in every recovered message at once let it advance past a message the run had not answered yet. So if the run answered the first recovered message, wrote its turn boundary, then crashed before dispatching the rest, the next boot resumed past those messages and they were never answered.Fix
A recovered message is now claimed on the session-stream router instead of folded into the cursor. A claim does two independent things:
The boot settles each claim as it dispatches the message, or right away for a message it folds into the seed chain or deliberately skips, so the cursor only advances over messages that have actually been handled. A claimed record whose route re-read never arrives over the tail degrades to being answered twice on the next boot, never to being dropped.
Covered by router-level unit tests for the claim/settle floor and a chat.agent boot test asserting the cursor published after the first recovered turn stays behind the still-unanswered ones.